edtlib: fix phandle edge circular dependency#106226
edtlib: fix phandle edge circular dependency#106226ifx-rmcs wants to merge 2 commits intozephyrproject-rtos:mainfrom
Conversation
|
Hi, if you'd like this to be merged for v4.4, please file a bug which explains the problem in more detail and mark this PR as fixing the bug. Otherwise I'll get to it after the merge window reopens next time. |
Added is_child helper to skip phandle dependency edges when the target is a descendent of the source node. This fixes a circular dependency that can occur when the dependency graph is built using tree edges (child depends on parent) and phandle edges (node - phandle target). i.e. (parent depends on child). When a parent references its own children using phandles, this circular dependency graph becomes problematic. The tree structure must always have a parent before child ordering. Since every parent already contains its child, skipping the edge addition is appropriate in this scenario. Signed-off-by: Richard Mc Sweeney <Richard.McSweeney@infineon.com>
Added a test to verify that phandle edge circular dependency no longer occurs when a parent references a child phandle. Signed-off-by: Richard Mc Sweeney <Richard.McSweeney@infineon.com>
ifx-rmcs
force-pushed
the
ifx_pse84_edtlib
branch
from
5e28971 to
4d93ac3
Compare
|
I've added a small test to verify that the circular dependency no longer occurs. I've also created #106576 bug ticket. |
How does that make any sense? it's a parent node, meaning by definition it shouldn't depend on a child node.
Why would it do that!? when it can just get that child by index, compatible, addr, node label, ... |
|
This is a generic solution built on the phy-handle fix that went it earlier for Ethernet. It's needed to handle cases such as ac-states in the autanalog feature being added in #106227 . |
| test_stm { | ||
| compatible = "test,stm"; | ||
| states = <&state1 &state2 &state3>; | ||
|
|
||
| state1: state1 { | ||
| next-state = <2>; | ||
| }; | ||
|
|
||
| state2: state2 { | ||
| next-state = <3>; | ||
| }; | ||
|
|
||
| state3: state3 { | ||
| next-state = <1>; | ||
| }; |
There was a problem hiding this comment.
I'm guessing that the test won't pass without the edtlib patch, but this is exactly the sort of thing that we intend to capture as a circular dependency. I know that there are some unpleasant limitations in our current approach, but this is not something I am open to revisiting in a piecemeal fashion, use-case-by-use-case, like is being done here.
For now I would suggest refactoring your DT to avoid this. If you want to re-start the larger conversation about how zephyr tracks DT dependencies, I would request that you file a much more detailed report and plan. It would be a stability and maintainability nightmare in my opinion if zephyr had subtle but minor differences in how it managed DT dependencies between releases, which is the path we are going down if we start to merge PRs like this one.
There was a problem hiding this comment.
Yes, I am currently working around this issue by placing my DT in a way that it doesn't encounter the circular dependency. This PR is to improve the end user experience. I figured that generalizing the existing solution for the Ethernet would be beneficial for others also.
There was a problem hiding this comment.
This doesn't improve the end user experience in my opinion. It seems to directly contradict our specification for how dependencies are supposed to work:
https://docs.zephyrproject.org/latest/build/dts/api/api.html#inter-node-dependencies
I understand your idea but please close this PR if you don't have the resources to try to tackle the more general problem
There was a problem hiding this comment.
It looks to me like the existing Ethernet phy-handle is a piecemeal or a case-by-case solution. Perhaps I don't understand the general issue related to circular dependency and the test I added is too specific? I can close the PR if there's some other bigger picture I'm not seeing.
There was a problem hiding this comment.
Part of the bigger picture that I'm referring to is that this PR seems to completely break the algorithm we have documented for how dependency tracking is meant to work. If that's not clear, let's just close this for now. Thanks for your contribution but changes to this code have a huge blast radius and I don't believe you've worked through the details enough for me to accept this. I appreciate your effort, though.
|
I'll refer here to the docs, reviewers are not meant to close PRs over technical disagreement. https://docs.zephyrproject.org/latest/contribute/reviewer_expectations.html#reviewer-expectations Please do not close a PR simply because you disagree. If we need to work here to make a different solution that is one thing. A NACK is understandable. But the message in closing here is "we don't want your patches" is really the wrong approach in my view and the docs agree. |
|
6 participants
This PR addresses #106576
Added is_child helper to skip phandle dependency edges when the target is a descendent of the source node. This fixes a circular dependency that can occur when the dependency graph is built using tree edges (child depends on parent) and phandle edges (node - phandle target). i.e. (parent depends on child).
When a parent references its own children using phandles, this circular dependency graph becomes problematic. The tree structure must always have a parent before child ordering. Since every parent already contains its child, skipping the edge addition is appropriate in this scenario.